[App Service] Easy Auth V2 Commands by mkarmark · Pull Request #3502 · Azure/azure-cli-extensions

mkarmark · 2021-06-11T23:49:58Z

This checklist is used to make sure that common guidelines for a pull request are followed.

General Guidelines

Have you run azdev style <YOUR_EXT> locally? (pip install azdev required)
Have you run python scripts/ci/test_index.py -q locally?

For new extensions:

My extension description/summary conforms to the Extension Summary Guidelines.

About Extension Publish

There is a pipeline to automatically build, upload and publish extension wheels.
Once your PR is merged into master branch, a new PR will be created to update src/index.json automatically.
The precondition is to put your code inside this repo and upgrade the version in the PR but do not modify src/index.json.

yonzhan · 2021-06-12T00:50:27Z

App Service

cgillum

A few commands and questions based on my review of the samples.

src/authV2/azext_authV2/_help.py

ConnorMcMahon

Overall I think it looks pretty good! Mostly some nitpicks and logistics questions.

src/authV2/azext_authV2/_client_factory.py

src/authV2/azext_authV2/_help.py

src/authV2/azext_authV2/_params.py

src/authV2/azext_authV2/azext_metadata.json

src/authV2/setup.py

src/authV2/azext_authV2/_validators.py

src/authV2/azext_authV2/custom.py

cgillum

A few more nits/questions, but I don't see anything fundamentally blocking. Great work on this!

src/authV2/setup.py

src/authV2/azext_authV2/_help.py

ConnorMcMahon

LGTM!

mkarmark · 2021-07-13T22:39:56Z

@Juliehzl @zhoxing-ms The only CI checks that are failing are the integrated tests for other extensions (not anything I touched). Is there any way to get passed those blockers if we want to release this extension soon?

Juliehzl · 2021-07-14T07:39:45Z

@Juliehzl @zhoxing-ms The only CI checks that are failing are the integrated tests for other extensions (not anything I touched). Is there any way to get passed those blockers if we want to release this extension soon?

Hi @mkarmark, please merge or rebase your PR with latest main branch.

mkarmark · 2021-07-14T16:58:41Z

#sign-off

mkarmark · 2021-07-14T17:06:12Z

@Juliehzl That worked, thanks! What steps are left for me to merge this PR?

Juliehzl · 2021-07-15T02:34:01Z

@Juliehzl That worked, thanks! What steps are left for me to merge this PR?

Please resolve conflicts to make CI pass. service name items need to be in alphabetical order

panchagnula · 2021-07-15T16:58:42Z

src/authV2/azext_authV2/azext_metadata.json

what is the reason for this? This will override all the webapp extension commands to not have preview tag - & that is not correct. Please verify & remove.

so this is a separate extension from the webapps extension and these commands are not preview, so I'm confused how it would override a diff extension commands?

src/authV2/azext_authV2/custom.py

panchagnula

Few comments to address.

scripts/ci/credscan/CredScanSuppressions.json

src/authV2/azext_authV2/azext_metadata.json

src/service_name.json

panchagnula

A few minor comments. otherwise looks good.

.github/CODEOWNERS

mkarmark · 2021-07-29T00:06:09Z

@Juliehzl @panchagnula my local test generates a recording that uses 2020-10-01 but the test expects api version 2021-04-01. Do you know what I can do to mitigate this error?

yonzhan · 2021-07-29T22:37:02Z

@panchagnula could you please help above question since Zunli is OOF?

panchagnula · 2021-07-29T23:14:48Z

@Juliehzl @panchagnula my local test generates a recording that uses 2020-10-01 but the test expects api version 2021-04-01. Do you know what I can do to mitigate this error?

@mkarmark as we discussed pleas wait for @calvinsID to update the CLI extension to use the track 2 SDK version & then re-base. Thanks!

panchagnula

LGTM

yonzhan requested a review from Juliehzl June 12, 2021 00:50

yonzhan assigned Juliehzl Jun 12, 2021

yonzhan requested a review from zhoxing-ms June 12, 2021 00:50

yonzhan added this to the S189 milestone Jun 12, 2021

cgillum reviewed Jun 17, 2021

View reviewed changes

ConnorMcMahon reviewed Jul 1, 2021

View reviewed changes

cgillum approved these changes Jul 2, 2021

View reviewed changes

ConnorMcMahon approved these changes Jul 12, 2021

View reviewed changes

cgillum approved these changes Jul 12, 2021

View reviewed changes

panchagnula reviewed Jul 15, 2021

View reviewed changes

src/authV2/azext_authV2/custom.py Outdated Show resolved Hide resolved

panchagnula reviewed Jul 15, 2021

View reviewed changes

src/authV2/azext_authV2/custom.py Outdated Show resolved Hide resolved

panchagnula reviewed Jul 15, 2021

View reviewed changes

src/authV2/azext_authV2/custom.py Outdated Show resolved Hide resolved

panchagnula suggested changes Jul 15, 2021

View reviewed changes

mkarmark requested a review from panchagnula July 15, 2021 23:01

panchagnula reviewed Jul 20, 2021

View reviewed changes

scripts/ci/credscan/CredScanSuppressions.json Outdated Show resolved Hide resolved

panchagnula reviewed Jul 20, 2021

View reviewed changes

src/authV2/azext_authV2/azext_metadata.json Outdated Show resolved Hide resolved

panchagnula reviewed Jul 20, 2021

View reviewed changes

src/service_name.json Outdated Show resolved Hide resolved

panchagnula suggested changes Jul 20, 2021

View reviewed changes

mkarmark commented Jul 28, 2021

View reviewed changes

.github/CODEOWNERS Outdated Show resolved Hide resolved

yonzhan assigned zhoxing-ms Jul 29, 2021

mkarmark added 27 commits July 29, 2021 20:33

oidc done

24bd0bb

more work

f1c66d2

secret prompts added

68c5c39

finish work

ee57ba3

style fixes

0ae2027

more style fixes

798a64e

pylint passed

846201f

address some of chris' comments

cb9bdef

address PR comments, clean up

97022e5

address some CI concerns

0ceb72f

more fixes

a8f0df1

linter should pass

63594de

fix codeowners

5be1ce4

linter should pass

ee4a9bd

linter should pass

7775937

really hope this passes the ci

bbbcf08

fix credscan

144bb36

address Sisira's comments

2208c78

fix static analysis

1e92776

address Sisira's comments

cfa6adb

try now

d732a0c

try now

5d0eda7

try now

092cba8

try now

66a5c0c

revert

4f6284e

please

4b5ce91

now

6ab5419

panchagnula removed the do-not-merge label Jul 30, 2021

panchagnula approved these changes Jul 30, 2021

View reviewed changes

panchagnula merged commit 8feab47 into Azure:main Jul 30, 2021

Conversation

mkarmark commented Jun 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

General Guidelines

About Extension Publish

Uh oh!

yonzhan commented Jun 12, 2021

Uh oh!

cgillum left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ConnorMcMahon left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cgillum left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ConnorMcMahon left a comment

Choose a reason for hiding this comment

Uh oh!

mkarmark commented Jul 13, 2021

Uh oh!

Juliehzl commented Jul 14, 2021

Uh oh!

mkarmark commented Jul 14, 2021

Uh oh!

mkarmark commented Jul 14, 2021

Uh oh!

Juliehzl commented Jul 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

panchagnula Jul 15, 2021

Choose a reason for hiding this comment

Uh oh!

mkarmark Jul 15, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

panchagnula left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

panchagnula left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mkarmark commented Jul 29, 2021

Uh oh!

yonzhan commented Jul 29, 2021

Uh oh!

panchagnula commented Jul 29, 2021

Uh oh!

panchagnula left a comment

Choose a reason for hiding this comment

mkarmark commented Jun 11, 2021 •

edited

Loading

Juliehzl commented Jul 15, 2021 •

edited

Loading